fix(lsp): fix incorrect project dir when configurationPath points to config outside of workspace#9392
Conversation
🦋 Changeset detectedLatest commit: 8bcd697 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
g-ortuno
changed the title
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new ConfigurationPathHint variant Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/evil-snails-arrive.md:
- Around line 1-13: The changeset front matter uses multiple "minor" entries and
a non-compliant description; update the .changeset/evil-snails-arrive.md front
matter to list only the affected package "@biomejs/biome" with change type
"patch" (remove the extra CLI package entries and change "minor" -> "patch"),
and replace the summary line with the required bug-fix format: start the
description with "Fixed [`#959`](https://github.com/biomejs/biome/issues/959): Fix
incorrect project dir when configurationPath points to config outside of
workspace". Ensure the file now follows the guideline of a single package entry
and a "Fixed [`#NUMBER`](...): ..." description.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5f30e477-d542-46c2-a82f-e72c0a0deb70
📒 Files selected for processing (3)
.changeset/evil-snails-arrive.mdcrates/biome_lsp/src/server.tests.rscrates/biome_lsp/src/session.rs
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.changeset/evil-snails-arrive.md (2)
1-11: Consider removing the CLI package entries.The coding guidelines specify "package (
@biomejs/biome)" (singular), suggesting only the main package should be listed. Since this LSP fix isn't platform-specific, you might simplify the front matter to just"@biomejs/biome": patch.♻️ Simplified front matter
--- "@biomejs/biome": patch -"@biomejs/cli-darwin-arm64": patch -"@biomejs/cli-darwin-x64": patch -"@biomejs/cli-linux-arm64": patch -"@biomejs/cli-linux-arm64-musl": patch -"@biomejs/cli-linux-x64": patch -"@biomejs/cli-linux-x64-musl": patch -"@biomejs/cli-win32-arm64": patch -"@biomejs/cli-win32-x64": patch ---As per coding guidelines, changeset files should specify the package (singular) and typically only list
@biomejs/biomeunless there are platform-specific changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.changeset/evil-snails-arrive.md around lines 1 - 11, The changeset front matter lists many platform-specific CLI packages (e.g. "@biomejs/cli-darwin-arm64", "@biomejs/cli-linux-x64", "@biomejs/cli-win32-arm64") but the guideline requires only the main package; update .changeset/evil-snails-arrive.md by removing all the "@biomejs/cli-*" entries and leave a single entry: "@biomejs/biome": patch so the front matter only references the main package.
13-13: Issue link format deviates slightly from the guideline.The coding guideline specifies
Fixed [#NUMBER](issue link): ...format, but you've used[biomejs/biome-vscode#959]. Since the issue is in a different repository, the full reference is arguably clearer, though it doesn't strictly match the guideline pattern.As per coding guidelines: "For bug fix changesets, start with 'Fixed [
#NUMBER](issue link): ...' format".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.changeset/evil-snails-arrive.md at line 13, Update the changeset header to follow the project's guideline by replacing the nonconforming "Fixed [biomejs/biome-vscode#959](https://github.com/biomejs/biome-vscode/issues/959): LSP now correctly resolves project directory when `configurationPath` points to a configuration file outside the workspace." with the required "Fixed [`#959`](https://github.com/biomejs/biome-vscode/issues/959): ..." format so the entry starts with "Fixed [`#959`](...):" while preserving the rest of the message and link; locate the string "Fixed [biomejs/biome-vscode#959]" to change it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.changeset/evil-snails-arrive.md:
- Around line 1-11: The changeset front matter lists many platform-specific CLI
packages (e.g. "@biomejs/cli-darwin-arm64", "@biomejs/cli-linux-x64",
"@biomejs/cli-win32-arm64") but the guideline requires only the main package;
update .changeset/evil-snails-arrive.md by removing all the "@biomejs/cli-*"
entries and leave a single entry: "@biomejs/biome": patch so the front matter
only references the main package.
- Line 13: Update the changeset header to follow the project's guideline by
replacing the nonconforming "Fixed
[biomejs/biome-vscode#959](https://github.com/biomejs/biome-vscode/issues/959):
LSP now correctly resolves project directory when `configurationPath` points to
a configuration file outside the workspace." with the required "Fixed
[`#959`](https://github.com/biomejs/biome-vscode/issues/959): ..." format so the
entry starts with "Fixed [`#959`](...):" while preserving the rest of the message
and link; locate the string "Fixed [biomejs/biome-vscode#959]" to change it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8b486e51-16f3-4330-a658-e4d6ac33a360
📒 Files selected for processing (1)
.changeset/evil-snails-arrive.md
|
@ematipico could you take a look? |
crates/biome_lsp/src/session.rs
Outdated
| } else { | ||
| path.to_path_buf() | ||
| } | ||
| // Collect workspace folder roots as absolute Utf8PathBufs. |
There was a problem hiding this comment.
Please revert all the code you added here. This isn't the right place. The code we had was only meant to normalize file VS folder.
There's method called resolve_configuration_path, that's where we want to add your fix
There was a problem hiding this comment.
Thanks for taking a look! How would we incorporate this logic into resolve_configuration_path? The comment for the method says "Resolves the user-provided configurationPath setting to an absolute path.", so returning the config path directly if the path is absolute makes sense to me.
The issue, as far as I can tell, it's not that we're resolving the config path incorrectly, rather than in this method, load_biome_configuration_file_internal, we're using the config path to open the project further below.
There was a problem hiding this comment.
The semantics of ConfigurationPathHint::FromUser is "take the path as it is (folder/file) and attempt to load it". No further manipulations should be done, as per semantics.
You might want to consider another variant for ConfigurationPathHint, so that we can explictily give proper semantics, and they don't clash with the existing ones.
There was a problem hiding this comment.
Double-checking if I understand the suggestion:
- We would change
resolve_configuration_path()to return a newConfigurationPathHint::FromUserExternal(config_path, project_root)for external configs. - In load_biome_configuration_file_internal, we'll use the FromUserExternal variant to load the config with
config_pathand open the project withproject_root.
Is that roughly what you had in mind?
There was a problem hiding this comment.
Yes that should do it.
So the function resolve_configuration_path() returns the new variant only if the path is outside of any working directory (workspace and base_path). Like the case we're trying to solve.
However I don't think the new variant needs to hold the project root. We already have the project root. We're only interested in loading the configuration from a different path.
There was a problem hiding this comment.
We already have the project root.
Would this be fs.working_directory()?
There was a problem hiding this comment.
I'm not sure, I can't remember at the moment. I believe it should be base_path or the current workspace directory. The key point is that the user opens the project at /Users/eva/www/project1/, while the configuration is at /var/common/biome.json. So we read the configuration at /var/common/biome.json but the project is still in the directory /Users/eva/www/project1/
There was a problem hiding this comment.
Ok! Ran out of time to clean things up and write a test for relative paths, but the current code compiles and the test passes. Let me know if you had something else in mind! I'll do another pass and write a test tomorrow.
There was a problem hiding this comment.
Ok, cleaned things up and added a test for relative external configs. I want to add one more test for workspaces, but please have a look in the meantime!
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.changeset/evil-snails-arrive.md (1)
5-5:⚠️ Potential issue | 🟡 MinorUse the required
[#NUMBER](issue link)format.Tiny changeset nit: the prose is fine, but the link text should be just the issue number, e.g.
Fixed [#959](...), notFixed [biomejs/biome-vscode#959](...). House style wins again.Suggested tweak
-Fixed [biomejs/biome-vscode#959](https://github.com/biomejs/biome-vscode/issues/959): LSP now correctly resolves project directory when `configurationPath` points to a configuration file outside the workspace. +Fixed [`#959`](https://github.com/biomejs/biome-vscode/issues/959): LSP now correctly resolves project directory when `configurationPath` points to a configuration file outside the workspace.As per coding guidelines: "For bug fix changesets, start with 'Fixed [
#NUMBER](issue link): ...' format".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.changeset/evil-snails-arrive.md at line 5, Replace the link text in the changeset intro so it matches the required format: find the string "Fixed [biomejs/biome-vscode#959](https://github.com/biomejs/biome-vscode/issues/959): ..." and change it to "Fixed [`#959`](https://github.com/biomejs/biome-vscode/issues/959): ..." so the link text is just the issue number as required by the bug-fix changeset style.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.changeset/evil-snails-arrive.md:
- Line 5: Replace the link text in the changeset intro so it matches the
required format: find the string "Fixed
[biomejs/biome-vscode#959](https://github.com/biomejs/biome-vscode/issues/959):
..." and change it to "Fixed
[`#959`](https://github.com/biomejs/biome-vscode/issues/959): ..." so the link
text is just the issue number as required by the bug-fix changeset style.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b9113a6b-9e47-4b68-9b70-f0e20fb44f80
📒 Files selected for processing (1)
.changeset/evil-snails-arrive.md
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/biome_lsp/src/session.rs (1)
719-759:⚠️ Potential issue | 🟠 MajorKeep in-workspace
configurationPaths on theFromUserpath.Line 719 only checks
workspace_folders, and Lines 752-759 always returnFromUserExternal. That means an in-project config can be misclassified as external when the client only sendsrootUri, or when start-up resolves a relative path against the first workspace. Downstream, that flips project resolution from the config’s parent tobase_path()/cwd, which is a regression for existing in-workspaceconfigurationPaths. Please only emitFromUserExternalonce the resolved absolute path actually sits outside every workspace root andbase_path().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_lsp/src/session.rs` around lines 719 - 759, The code currently emits ConfigurationPathHint::FromUserExternal in several fallbacks without verifying the resolved absolute path is actually outside every workspace root and the server base_path; update the logic around config_path resolution (symbols: config_path, workspace_roots, file_path, root.join(...), ConfigurationPathHint::FromUser, ConfigurationPathHint::FromUserExternal, self.base_path()) so that every time you resolve an absolute path (whether from an absolute config_path, joined against the best-matching workspace root, against workspace_roots.first(), or against self.base_path()), you check if that resolved absolute path starts_with any workspace_roots or starts_with the base_path() and only return FromUserExternal when it truly sits outside all workspace roots and base_path; otherwise return FromUser. Ensure the file-path branch (the joined variable) and the fallback branches use this same containment test before choosing FromUser vs FromUserExternal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_lsp/src/session.rs`:
- Around line 1050-1055: The ConfigurationPathHint::FromUserExternal branch
currently chooses base_path() or fs.working_directory() and skips workspace
folders; change it to prefer the first workspace folder if present (e.g., check
self.workspace_folders or the same accessor used elsewhere), then fall back to
self.base_path(), and finally to fs.working_directory() so external configs use
the workspace root and did_open can locate files correctly; update the branch in
session.rs where ConfigurationPathHint::FromUserExternal is matched to implement
that ordering.
---
Outside diff comments:
In `@crates/biome_lsp/src/session.rs`:
- Around line 719-759: The code currently emits
ConfigurationPathHint::FromUserExternal in several fallbacks without verifying
the resolved absolute path is actually outside every workspace root and the
server base_path; update the logic around config_path resolution (symbols:
config_path, workspace_roots, file_path, root.join(...),
ConfigurationPathHint::FromUser, ConfigurationPathHint::FromUserExternal,
self.base_path()) so that every time you resolve an absolute path (whether from
an absolute config_path, joined against the best-matching workspace root,
against workspace_roots.first(), or against self.base_path()), you check if that
resolved absolute path starts_with any workspace_roots or starts_with the
base_path() and only return FromUserExternal when it truly sits outside all
workspace roots and base_path; otherwise return FromUser. Ensure the file-path
branch (the joined variable) and the fallback branches use this same containment
test before choosing FromUser vs FromUserExternal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 12731298-7c14-4a4b-82a7-f1ba6c9e0c87
📒 Files selected for processing (4)
crates/biome_configuration/src/lib.rscrates/biome_lsp/src/handlers/text_document.rscrates/biome_lsp/src/session.rscrates/biome_service/src/configuration.rs
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/biome_lsp/src/handlers/text_document.rs (1)
45-53:⚠️ Potential issue | 🟠 MajorDon't make external configs guess the project root.
This branch has the document path, but it only passes the config hint downstream. For
FromUserExternal, that means the loader has to guess a root later, so a shared config can still register the wrong project in multi-root workspaces.Please keep the per-file project path alongside the hint here, rather than recomputing it later from session-wide fallbacks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_lsp/src/handlers/text_document.rs` around lines 45 - 53, The code calls session.resolve_configuration_path(...) and then only passes the config hint into session.load_biome_configuration_file(...), which forces FromUserExternal to guess the project root later; instead preserve and pass the per-file project path (the resolved document/project path) alongside the configuration hint to load_biome_configuration_file (or its caller) so the loader can use the explicit per-file project path rather than recomputing via session-wide fallbacks; update the call sites around session.resolve_configuration_path, session.load_biome_configuration_file, and any FromUserExternal loader invocation to accept and propagate the per-file project path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_lsp/src/session.rs`:
- Around line 719-725: The closure hint_for_path currently only checks
workspace_roots when deciding between ConfigurationPathHint::FromUser and
::FromUserExternal, which mislabels paths under base_path() in single-root
sessions; update the predicate to consider base_path() as a fallback workspace
root (e.g., treat path as "FromUser" if it starts_with any workspace_roots OR
starts_with(base_path())), referencing the hint_for_path closure,
workspace_roots, base_path(), and the ConfigurationPathHint variants to locate
where to change the check.
---
Duplicate comments:
In `@crates/biome_lsp/src/handlers/text_document.rs`:
- Around line 45-53: The code calls session.resolve_configuration_path(...) and
then only passes the config hint into
session.load_biome_configuration_file(...), which forces FromUserExternal to
guess the project root later; instead preserve and pass the per-file project
path (the resolved document/project path) alongside the configuration hint to
load_biome_configuration_file (or its caller) so the loader can use the explicit
per-file project path rather than recomputing via session-wide fallbacks; update
the call sites around session.resolve_configuration_path,
session.load_biome_configuration_file, and any FromUserExternal loader
invocation to accept and propagate the per-file project path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b3a12afd-55ff-4f42-9c81-3bac9aed2eba
📒 Files selected for processing (3)
crates/biome_lsp/src/handlers/text_document.rscrates/biome_lsp/src/server.tests.rscrates/biome_lsp/src/session.rs
| let hint_for_path = |path: Utf8PathBuf| { | ||
| Some(if workspace_roots.iter().any(|r| path.starts_with(r)) { | ||
| ConfigurationPathHint::FromUser(path) | ||
| } else { | ||
| ConfigurationPathHint::FromUserExternal(path) | ||
| }) | ||
| }; |
There was a problem hiding this comment.
Treat base_path() as the fallback workspace root.
When workspace_folders is empty, this closure still only checks workspace_roots, so any path resolved under base_path() is mislabelled FromUserExternal. That flips valid rootUri-relative configurationPaths onto the external path in single-root sessions.
💡 Suggested tweak
+ let fallback_root = if workspace_roots.is_empty() {
+ self.base_path()
+ } else {
+ None
+ };
+
let hint_for_path = |path: Utf8PathBuf| {
- Some(if workspace_roots.iter().any(|r| path.starts_with(r)) {
+ let is_internal = workspace_roots.iter().any(|root| path.starts_with(root))
+ || fallback_root
+ .as_ref()
+ .is_some_and(|root| path.starts_with(root));
+
+ Some(if is_internal {
ConfigurationPathHint::FromUser(path)
} else {
ConfigurationPathHint::FromUserExternal(path)
})
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let hint_for_path = |path: Utf8PathBuf| { | |
| Some(if workspace_roots.iter().any(|r| path.starts_with(r)) { | |
| ConfigurationPathHint::FromUser(path) | |
| } else { | |
| ConfigurationPathHint::FromUserExternal(path) | |
| }) | |
| }; | |
| let fallback_root = if workspace_roots.is_empty() { | |
| self.base_path() | |
| } else { | |
| None | |
| }; | |
| let hint_for_path = |path: Utf8PathBuf| { | |
| let is_internal = workspace_roots.iter().any(|root| path.starts_with(root)) | |
| || fallback_root | |
| .as_ref() | |
| .is_some_and(|root| path.starts_with(root)); | |
| Some(if is_internal { | |
| ConfigurationPathHint::FromUser(path) | |
| } else { | |
| ConfigurationPathHint::FromUserExternal(path) | |
| }) | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/biome_lsp/src/session.rs` around lines 719 - 725, The closure
hint_for_path currently only checks workspace_roots when deciding between
ConfigurationPathHint::FromUser and ::FromUserExternal, which mislabels paths
under base_path() in single-root sessions; update the predicate to consider
base_path() as a fallback workspace root (e.g., treat path as "FromUser" if it
starts_with any workspace_roots OR starts_with(base_path())), referencing the
hint_for_path closure, workspace_roots, base_path(), and the
ConfigurationPathHint variants to locate where to change the check.
There was a problem hiding this comment.
@g-ortuno I think this is relevant. What do you think?
There was a problem hiding this comment.
The more I look at this the more confused I get 😵💫. I'm not sure what the right behavior should be here. Imagine:
configuration path: "configs/biome.js"
base_path(): c:/project/
Resolved path: c:/project/configs/biome.js
The resolved path is inside base_path.
If we return FromUser(), then the project root will be c:/project/configs. If we return FromUserExternal(), then the project root will be c:/project/. So technically, the config is not external, but the chosen project root is correct.
I'm inclined to keep returning FromUserExternal() here. I can follow up with closely analyzing all the cases (I counted 15!), and proposing a more hollistic fix. What do you think?
There was a problem hiding this comment.
Yeah let's keep it as is. We'll keep it monitored! thank you very much for the fix, lot of users will be grateful
ematipico
left a comment
ematipico
left a comment
There was a problem hiding this comment.
Code looks good! Just one minor comment to address
Merging this PR will not alter performance
Comparing 
Footnotes
|
2 participants
I used AI to find the issue, write the fix, and an initial version of the test. I ran out of tokens, so I rewrote the test myself (since it wasn't actually verifying anything) and verified it fails without the fix and passes with the fix. I'm mainly a C++ developer so happy to address any idiomatic rust feedback!
Summary
Closed biomejs/biome-zed#108
#9049 fixed #7138 but inadvertently broke projects where
configurationPathpointed to a folder outside of the workspace. biomejs/biome-vscode#959 and #9217 are related.The repro case is something like this:
Before #9049, in
load_biome_configuration_file_internal, settingconfigurationPathin VSCode would fall back to the_case, which would usebase_path()aspathwhich is later used to open the project. IIUC,base_pathwould be the workspace's path.After #9049, the new branch runs which uses the parent of the configuration file which in the repro case is c:/code/configs/.
I added a log after this function and I see the following, which confirms the above.
I also added a log to text_document.rs in did_open() in session.project_for_path(). So when I try to format a file I see the following in the logs:
The PR fixes this by not using the config's parent as the root of the project; instead if the config's parent is outside the workspaces, use the first workspace or fall back to
base_path.Test Plan
Verified locally with configurationPath pointing to an external config that files in the workspace were formatted. I also added a test that reproduces the issue and verified it passes with the fix and fails without it.
Docs
N/A